[androiddebugbridge] Retry a command once when standby rejects the shell stream - #21247
Conversation
…ell stream Socket.isConnected() only records that a connection was once established; it stays true after the device stops serving the socket. A device in standby keeps the TCP connection but refuses to open the adb shell stream, so the reconnect check in handleCommand cannot notice it and the command fails on stream open. The command is then dropped, which most often loses the very KEYCODE_WAKEUP meant to end standby - sending it twice is a common workaround. Throw a distinct AndroidDebugBridgeDeviceStreamRejectedException for that case and, when it occurs, reconnect and run the command once more. The exception is only raised where the stream never opened, so the command provably did not run and retrying it cannot execute it twice. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Thanks for working on this. The reconnect-and-retry approach makes sense for devices that reject the first shell stream while waking from standby, but I think the retry condition is currently broader than is safe.
The new exception is created for every captured streamError, while those errors do not necessarily prove that the command was rejected before execution. Since handleCommand() retries the complete original command, this can potentially duplicate non-idempotent operations.
I think the retry should be limited to failures where we can reliably establish that the command was not executed, or alternatively to commands that are explicitly safe to repeat.
Opening the shell stream failing does not prove the device never got the request. adblib writes the OPEN packet inside AdbConnection.open(), so an IOException raised while sending it leaves delivery ambiguous, and the previous code also captured failures from the read loop, which happen after the command already ran. Retrying on those could run a command twice. Split the two phases so only a failed open raises the stream-rejected type, and gate the retry on commands that stay correct when executed twice: plain reads and the wake-up key event this recovery exists for. Text, taps, media, packages, intents and shutdown are no longer repeated. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous retry-safety concern. Separating failures while opening the stream from failures while reading it, and restricting retries to commands intended to be safe to repeat, is a good improvement.
AI re-reviewed the current changes and found two remaining cases that could still cause incorrect behavior:
RefreshTypeis currently considered safe regardless of channel, although not every channel treatsREFRESHas a read-only operation.- The reconnect/retry sequence is performed after the device command lock has been released, so it can race with another shell operation and cancel that operation through
disconnect().
I think these should be addressed before merging.
…onnect REFRESH is only handled as a read on some channels. Elsewhere it is not special cased and ends up as a value, so a REFRESH on the text channel runs "input text REFRESH" and record-input acts, neither of which may be repeated. Only treat REFRESH as safe on the channels that read state. The reconnect also ran after runAdbShell had released the command lock, so another operation could start a shell command in the gap and have its future cancelled by disconnect(), making its get() throw CancellationException in a caller that does not expect one -- checkConnection() would have died on it. Move the reconnect into the device and take the command lock for it. Plain disconnect() still aborts running commands, which its other callers rely on. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous findings. The channel-specific REFRESH handling and serialized reconnect resolve those concerns.
AI found one remaining concurrency issue: runAdbShell() captures the current AdbConnection before acquiring commandLock. A concurrent retry reconnect can therefore replace and close that connection while another operation is waiting for the lock, after which the waiting operation proceeds using the stale connection.
This should be addressed before merging; moving the connection lookup under the command lock looks like the simplest fix.
runAdbShell captured the AdbConnection before taking commandLock, so a retry reconnect could replace and close it while another call was still waiting for the lock. That call then ran against a connection that was already closed, which for a non-repeatable command surfaced as a spurious stream-open failure and made handleCommand disconnect the newly established one. The connection checker could hit the same stale reference. Take the reference only once the lock is held, which is what makes the serialization in reconnectForRetry effective. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
|
the connection is read under the command lock now, so a reconnect cannot swap it out while another call is waiting for the lock. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous finding. The stale connection race is fixed.
AI found one remaining concurrency issue: reconnectForRetry() is protected by commandLock, but normal connect() calls are not. Another thread can therefore start connect() during the retry reconnect and, because connect() begins with disconnect(), interfere with the connection being established.
This should be addressed before merging.
The PR description should also be updated because it still says a stream-open failure proves the command was never executed, while the implementation now correctly treats delivery as potentially ambiguous.
connect() begins by disconnecting, so a call from handleCommand or the connection checker could tear down the connection reconnectForRetry was still establishing. Take the command lock for connect() as well, which also makes reconnectForRetry simply a serialized connect. disconnect() stays outside the lock: aborting a running command is what disconnectOnMaxADBTimeouts and dispose need from it. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
|
connect() takes the command lock now as well, so a connect from handleCommand or the checker cannot tear down what the retry reconnect is establishing, since connect starts with a disconnect. disconnect itself stays outside the lock because disconnectOnMaxADBTimeouts and dispose need it to abort a running command. also updated the description, it still said a stream open failure proves the command never ran and that is not what the code assumes anymore. |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous concurrency finding. Serializing ordinary connect() calls fixes the race with reconnectForRetry(), and the updated retry restrictions and PR description now match the ambiguous ADB OPEN delivery semantics.
AI found one remaining lifecycle/concurrency issue: connect() now waits for commandLock using non-interruptible lock(). dispose() cancels the scheduled connection checker with interruption before disconnecting, so a checker that is already waiting here will not abort its lock wait. Once the lock becomes available, it can still enter connectInternal() and start establishing a connection after the handler has been disposed.
This is important because cancellation during disposal should stop pending connection work rather than allow it to resume afterward and recreate resources that were just cleaned up.
Using an interruptible lock acquisition looks appropriate here, especially since connect() already declares InterruptedException.
dispose() cancels the connection checker with an interrupt and then disconnects, but a checker already blocked on the non-interruptible lock() did not abort. It could acquire the lock afterwards and build a fresh socket and ADB connection after the handler had been torn down. Use lockInterruptibly() so cancellation stops a pending attempt. Both callers already declare InterruptedException. runAdbShell takes the same lock and has the same problem, so it gets the same treatment: an interrupted command must not resume and run against the device after disposal. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
|
switched to lockInterruptibly so cancelling during dispose actually aborts a pending attempt instead of it resuming afterwards and building a new connection. i did the same in runAdbShell since it takes the same lock and has the same problem, an interrupted command should not resume and run against the device after disposal either. both already declared InterruptedException so nothing else changed. |
|
AI re-reviewed the current changes after the latest lifecycle/concurrency fix. The switch to A human maintainer review is still needed. |
AI review findings have been addressed. A human maintainer review is still needed.
lsiepel
left a comment
lsiepel
left a comment
There was a problem hiding this comment.
I only have one comment on this PR. There are quite a few verbose inline comments that clutter the code. The code is already clear and self-explanatory, so I’d prefer comments to be reserved for specific edge cases or non-obvious behavior that isn’t apparent from reading the code itself.
The same applies to the Javadocs—they’re more verbose than necessary and could be simplified considerably.
Otherwise LGTM
Keep only what is not evident from the code: the ambiguous OPEN delivery that limits which commands may be repeated, why disconnect() stays outside the lock, and why the lock waits are interruptible. Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
|
@lsiepel you are right, i went too far the other way. the review rounds here kept asking why a change was safe and i ended up writing all of that into the code, it was 80 comment lines on 163 insertions which is silly. trimmed it to about half and kept only the things you cannot see from the code, the ambiguous OPEN delivery that decides which commands may be repeated, why disconnect stays outside the lock, and why the lock waits are interruptible. the rest of the reasoning is in the commit messages and the description where it belongs. javadocs shortened too. |
…ell stream (openhab#21247) * [androiddebugbridge] Retry a command once when standby rejects the shell stream Signed-off-by: Stamate Viorel <stamate.viorel@gmail.com> Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
3 participants
Problem
Socket.isConnected()only records that a connection was once established — it staystrueafter the peer stops serving the socket. A device in standby keeps the TCP connection up but refuses to open the adb shell stream, so the reconnect check at the top ofhandleCommandsees a "connected" socket, skips the reconnect, and the command fails on stream open:The command is then dropped. In practice that most often loses the
KEYCODE_WAKEUPmeant to end standby, so the device doesn't wake and the fix looks like "send it twice" — the first send is spent discovering the connection was stale, and only the second one, which reconnects, lands.Change
runAdbShellnow separates the two failure phases. Only a failure while opening the stream raises the distinctAndroidDebugBridgeDeviceStreamRejectedException; a failure while reading an already-open stream stays an ordinaryAndroidDebugBridgeDeviceException, because by then the command has certainly reached the device.handleCommandcatches the first case, reconnects and runs the command once more.A failed open is not proof that the device never saw the request: adblib writes the OPEN packet inside
AdbConnection.open(), so anIOExceptionraised while sending it leaves delivery ambiguous. The retry is therefore restricted to commands that stay correct if they run twice — aREFRESHon the channels that handle it as a read, and the wake-up key event this recovery exists for, which is idempotent since waking an already awake device does nothing. Text, taps, media control, package and intent commands and shutdown are never repeated.The reconnect is serialized with the shell commands and with ordinary
connect()calls through the existing command lock, andrunAdbShellreads the connection only once that lock is held, so a reconnect cannot replace the connection a waiting call is about to use.disconnect()stays outside the lock, since aborting a running command is exactly what its other callers need.Testing
Verified on a Vestel Android TV that rejects the stream intermittently in standby: over repeated standby→wake cycles a single
KEYCODE_WAKEUPnow wakes it every time, and the retry is observably exercised when the reject occurs:Follow-up to #21222, which made this reject catchable instead of escaping the scheduler; this makes it recoverable.